fix(validators): reject valueHint on named arguments - #1339
Conversation
valueHint identifies a positional slot for transport URL variable substitution, so it is only valid on positional arguments. The model carries ValueHint flat on Argument and validateArgument never checked it for named arguments, and the JSON schema does not catch it (the argument union branches do not set additionalProperties: false), so a named argument with a valueHint validated. Enforce it in the semantic validator with ErrValueHintOnNamedArgument (code valuehint-on-named-argument), with tests both directions. Closes modelcontextprotocol#662.
JosephDoUrden
left a comment
JosephDoUrden
left a comment
There was a problem hiding this comment.
I checked out the branch, ran go test -count=1 ./internal/validators/... and everything passes, and it merges clean onto current main. I also verified the schema claim in the description: in every bundled schema version that has valueHint it is only declared on PositionalArgument, and because Argument is an anyOf union without additionalProperties: false, a named argument with a valueHint still passes schema validation. A quick probe with ValidationAll confirms this at runtime, the only issue reported is the new semantic one. Worth noting the publish handler only runs ValidationSchemaVersionAndSemantic anyway, so the semantic validator is the right layer for this. Seed data has no named argument with a valueHint, so nothing existing breaks. Tiny nit: the new tests only put the argument in runtimeArguments, the same function covers packageArguments so the check works there too, but one more case would not hurt. Heads up that this conflicts a bit with my open #1583 in validateArgument, whichever goes second needs a small manual fix, happy to handle it from my side. Looks merge-ready to me.
2 participants
Summary
Closes #662.
valueHintidentifies a positional slot for transport URL variable substitution, so it belongs only on positional arguments. Right now nothing stops a named argument from carrying one: the model holdsValueHintflat onArgument(pkg/model/types.go), andvalidateArgumentchecks the name and value fields for named arguments but not the hint.The JSON schema does not catch it either.
valueHintis declared onPositionalArgumentonly, butArgumentis ananyOfunion and neither branch setsadditionalProperties: false, so a named argument with avalueHintstill validates against theNamedArgumentbranch. That leaves the semantic validator as the enforcement point.Change
ErrValueHintOnNamedArgument.validateArgument, reject a non-emptyValueHinton a named argument (codevaluehint-on-named-argument), beside the existing named-argument checks.valueHintis rejected; a positional argument with avalueHintstays valid.The check is additive and matches the schema's positional-only declaration, so existing valid data is unaffected. I kept this at the validator level rather than tightening the generated schema, which felt like the smaller change; happy to put
additionalProperties: falseon the argument branches instead if you'd rather the schema carry it.Test
go test ./internal/validators/...passes.